Skip to content

Share the configcheck abnormal-failure retry and cover it - #2313

Merged
csatib02 merged 5 commits into
masterfrom
fix/configcheck-followups
Aug 10, 2026
Merged

Share the configcheck abnormal-failure retry and cover it#2313
csatib02 merged 5 commits into
masterfrom
fix/configcheck-followups

Conversation

@csatib02

@csatib02 csatib02 commented Aug 10, 2026

Copy link
Copy Markdown
Member

Summary

Follow-ups from the review of #2303, which landed fluentd.configCheckPod. Five small commits, one behaviour change.

The Status.Reason != "" retry branch — the one that tells a configcheck pod killed around the check apart from one that proved the config invalid — was copy-pasted byte-identically into both aggregators, along with the stillRunningContainer helper behind its log line. A later fix to those semantics would have landed in one copy only, and pkg/resources/syslogng has no test file at all, so its half shipped unexercised. Both move to pkg/resources/configcheck, which already owns the shared configcheck concerns, and get a table that states the rule: an invalid config is the only failure that is a verdict, and it is the one that leaves the pod-level Reason empty.

The one behaviour change: a PVC-backed extraVolume is still mounted as an empty dir on the configcheck pod — its claim only exists as a volumeClaimTemplate on the StatefulSet — but it now says so. It was the only substitution in that loop that happened silently, so a dry-run reading the mounted path failed and the operator reported the config invalid with nothing pointing at the swap. The predicate behind it is now shared with statefulset(), so the two paths cannot disagree about which volumes are template-backed.

The rest is test hardening. Each replaced test passed when the code it claimed to pin was reverted; each new one was verified to fail:

Reverted Was Now
Reason != ""== "DeadlineExceeded" green Evicted, NodeAffinity, Shutdown red
merge.Merge → direct assignment green on the nil-override row EmptyOverride + both override rows red
drop the spec-empty half of the PVC guard green ExistingClaimIsKept red
disable the PVC fallback red red

No API change, so no regeneration.

Test plan

  • make lint — 0 issues across all three modules
  • go test -race ./pkg/resources/...
  • Confirm each commit builds on its own: git rebase --exec 'go build ./pkg/resources/...' origin/master
  • CI e2e — the configcheck suites (TestFluentdAggregator_ConfigChecks*, TestSyslogNGIsRunningAndForwardingLogs) are the ones that exercise this path

…package

Both aggregators carried a byte-identical copy of the branch that decides a
failed configcheck pod was killed around the check rather than by an invalid
config, deletes it, and reports the retry - plus a copy of the
stillRunningContainer helper behind its log line. A later fix to those
semantics would have landed in one copy only.

pkg/resources/configcheck already owns the concerns both configchecks share,
so the branch moves there and each caller keeps only the result mapping it
owns. No behaviour change: same condition, same log, same delete, same
message.

Signed-off-by: Bence Csati <bence.csati@axoflow.com>
pkg/resources/syslogng has no test file at all, so its half of this logic
shipped unexercised; covering the shared function covers both callers.

The table states the rule rather than the implementation: an invalid config
is the only failure that is a verdict, and it is the one that leaves the
pod-level Reason empty. stillRunningContainer gets its own table - neither of
its branches had ever executed - including the case it exists for, a native
sidecar still running as an init container after the dry-run has finished.

Signed-off-by: Bence Csati <bence.csati@axoflow.com>
The test for "this extraVolume is only ever realized as a volumeClaimTemplate"
was written inline, so the configcheck pod - which cannot use such a volume -
had no way to ask the question without restating it. Name it next to the
emptiness check it wraps. Behaviour is unchanged; statefulset() branches on
the same condition it did before, negated.

Signed-off-by: Bence Csati <bence.csati@axoflow.com>
…heck pod

A PVC-backed extraVolume is mounted as an empty dir on the configcheck pod,
because the claim behind it only exists as a volumeClaimTemplate on the
StatefulSet. That was the one substitution in this loop that happened
silently: a dry-run that reads the mounted path then fails, and the operator
reports the config invalid with nothing pointing at the swap. Say what was
substituted and how to override it.

The two apply-and-log paths collapse back into one now that the volume to
mount is chosen first, and the condition comes from statefulset(), so the two
cannot disagree about which volumes are template-backed.

Signed-off-by: Bence Csati <bence.csati@axoflow.com>
Each of these was verified to fail when the code it pins is reverted, which
the tests they replace did not:

- the verdict test only covered DeadlineExceeded, the case that already worked
  before the condition was widened to any pod-level Reason, so narrowing it
  back left the suite green. It is now a table over Evicted, NodeAffinity and
  Shutdown as well, plus the empty-Reason case that must stay a verdict.
- the additive test asserted that a pod contains its own entries when no
  override is set, which cannot fail. It compares the whole pod spec instead,
  which is the actual backwards-compatibility claim, and covers an explicitly
  empty override next to the nil one.
- the extraVolume test asserted the volume source only, so dropping half the
  guard - the half that keeps a claim the user brought themselves - stayed
  green. Both claim shapes are now rows, and both assert the mount.

newCheckPodReconciler takes a client so the configCheck tests can use the
fixture the newCheckPod tests use, instead of re-inlining it.

Signed-off-by: Bence Csati <bence.csati@axoflow.com>
@csatib02 csatib02 self-assigned this Aug 10, 2026
@csatib02 csatib02 added the enhancement New feature or request label Aug 10, 2026
@csatib02
csatib02 requested a review from OverOrion August 10, 2026 14:25
@csatib02
csatib02 merged commit 7cf7d1e into master Aug 10, 2026
33 checks passed
@csatib02
csatib02 deleted the fix/configcheck-followups branch August 10, 2026 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants